Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Arc - Validate whether interceptors declare producer methods #31348

Merged
merged 2 commits into from
Feb 23, 2023
Merged

Arc - Validate whether interceptors declare producer methods #31348

merged 2 commits into from
Feb 23, 2023

Conversation

manovotn
Copy link
Contributor

Part of #28558

Simple fix that validated interceptors during InterceptorInfo creation to validate whether there are any producer methods/fields and throws definition exception if that's the case.

The exception looks something like this:

ProducerMethodInInterceptorTest.testFailure » Definition An interceptor method cannot me marked @Produces - java.lang.String generateString() in class: io.quarkus.arc.test.producer.illegal.ProducerMethodInInterceptorTest$BadInterceptor

@manovotn manovotn requested review from Ladicek and mkouba February 22, 2023 16:16
@quarkus-bot quarkus-bot bot added the area/arc Issue related to ARC (dependency injection) label Feb 22, 2023
@manovotn manovotn mentioned this pull request Feb 22, 2023
Closed
@manovotn
Copy link
Contributor Author

manovotn commented Feb 22, 2023 •
edited
Loading

Actually, just noticed that there is also a disposer method validation, I'll add that as well momentarily.

EDIT: Disposer method presence is now detected as well.

@manovotn manovotn force-pushed the validateProducersInInterceptors branch from d46b786 to b3671d1 Compare February 22, 2023 16:22
@quarkus-bot

This comment has been minimized.

Sign in to view
@mkouba mkouba added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Feb 23, 2023
Ladicek
Ladicek approved these changes Feb 23, 2023
View reviewed changes
Copy link
Contributor

@Ladicek Ladicek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the rest of InterceptorInfo ignores the AnnotationStore as well, I think we can merge this as is and fix it all in a subsequent PR.

@mkouba
Copy link
Contributor

mkouba commented Feb 23, 2023

Since the rest of InterceptorInfo ignores the AnnotationStore as well, I think we can merge this as is and fix it all in a subsequent PR.

I agree that in general we should query the AnnotationStore where possible. But this particular use case is a good example of a situation where it does not make much sense, i.e. a quarkus extension would have to modify annotations of an interceptor class and for some reason add @Produces there. So yes, it would be consistent but it does not bring a real benefit IMO.

BTW I wonder if Weld does throw a definition exception if a portable extension modifies an interceptor class and adds @Produces via ProcessAnnotatedType?

@Ladicek
Copy link
Contributor

Ladicek commented Feb 23, 2023

It's not just @Produces and others added in this PR -- the entire InterceptorInfo constructor ignores the annotation store. It looks for annotations like @AroundInvoke directly on the Jandex objects. I agree it's super-unlikely anyone would ever transform annotations on an interceptor class, but we should just be consistent in my opinion.

@mkouba
Copy link
Contributor

mkouba commented Feb 23, 2023

It's not just @Produces and others added in this PR -- the entire InterceptorInfo constructor ignores the annotation store. It looks for annotations like @AroundInvoke directly on the Jandex objects. I agree it's super-unlikely anyone would ever transform annotations on an interceptor class, but we should just be consistent in my opinion.

+1 but it's IMO a very low priority.

@manovotn manovotn force-pushed the validateProducersInInterceptors branch from b3671d1 to 4c25b68 Compare February 23, 2023 10:03
@manovotn
Copy link
Contributor Author

@Ladicek I've added a commit that delegates annotation queries to the AnnotationStore, please take a look again.

Ladicek
Ladicek approved these changes Feb 23, 2023
View reviewed changes
Copy link
Contributor

@Ladicek Ladicek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for sure :-)

@quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot
Copy link

quarkus-bot bot commented Feb 23, 2023

Failing Jobs - Building 174b2c6

Status Name Step Failures Logs Raw logs
✖ Maven Tests - JDK 11 Build Failures Logs Raw logs
✖ Maven Tests - JDK 11 Windows Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ Maven Tests - JDK 11 #

- Failing: integration-tests/maven

📦 integration-tests/maven

✖ io.quarkus.maven.it.BuildIT.testClassLoaderLinkageError line 102 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 0

✖ io.quarkus.maven.it.BuildIT.testClassLoaderLinkageError line 102 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 0

⚙️ Maven Tests - JDK 11 Windows #

- Failing: integration-tests/maven

📦 integration-tests/maven

✖ io.quarkus.maven.it.BuildIT.testClassLoaderLinkageError line 102 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 0

✖ io.quarkus.maven.it.BuildIT.testClassLoaderLinkageError line 102 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: 

expected: 0

@manovotn
Copy link
Contributor Author

io.quarkus.maven.it.BuildIT.testClassLoaderLinkageError failure is unrelated as I've seen it in other PRs as well.

@manovotn manovotn merged commit 1b8fc61 into quarkusio:main Feb 23, 2023
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Feb 23, 2023
@manovotn manovotn deleted the validateProducersInInterceptors branch February 23, 2023 23:58
@quarkus-bot quarkus-bot bot added this to the 3.0 - main milestone Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkouba mkouba mkouba approved these changes

@Ladicek Ladicek Ladicek approved these changes

Assignees
No one assigned
Labels
area/arc Issue related to ARC (dependency injection)
Projects
None yet
Milestone
3.0.0.Alpha5
Development

Successfully merging this pull request may close these issues.
3 participants
@manovotn @mkouba @Ladicek